Skip to content

Cast pins to signed integers to avoid Wtype-limits compile warning #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 16, 2019
Merged

Cast pins to signed integers to avoid Wtype-limits compile warning #95

merged 2 commits into from
Sep 16, 2019

Conversation

jrowberg
Copy link
Contributor

Using the current stock SoftwareSerial with all warnings enabled results in messages such as the following, observed in a Travis CI build:

In file included from /home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/cores/arduino/Arduino.h:257:0,
                 from /home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/libraries/SoftwareSerial/src/SoftwareSerial.cpp:43:
/home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/libraries/SoftwareSerial/src/SoftwareSerial.cpp: In member function 'void SoftwareSerial::begin(long int)':
/home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/variants/standard/pins_arduino.h:74:39: warning: comparison is always true due to limited range of data type [-Wtype-limits]
 #define digitalPinToPCICR(p)    (((p) >= 0 && (p) <= 21) ? (&PCICR) : ((uint8_t *)0))
                                       ^
/home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/libraries/SoftwareSerial/src/SoftwareSerial.cpp:319:7: note: in expansion of macro 'digitalPinToPCICR'
   if (digitalPinToPCICR(_receivePin)) {
       ^
/home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/variants/standard/pins_arduino.h:74:39: warning: comparison is always true due to limited range of data type [-Wtype-limits]
 #define digitalPinToPCICR(p)    (((p) >= 0 && (p) <= 21) ? (&PCICR) : ((uint8_t *)0))
                                       ^
/home/travis/.arduino15/packages/arduino/hardware/avr/1.6.23/libraries/SoftwareSerial/src/SoftwareSerial.cpp:360:6: note: in expansion of macro 'digitalPinToPCICR'
     *digitalPinToPCICR(_receivePin) |= _BV(digitalPinToPCICRbit(_receivePin));
      ^

This is due to the fact that SoftwareSerial uses the digitalPinToPCICR macro with an argument that is an unsigned data type, specifically uint8_t. The >= 0 comparison logically always evaluates to true, generating the warning shown here.

To solve this, because I hate compiler warnings being generated for no good reason, I've modified the AVR core SoftwareSerial.cpp file to explicitly cast the two offending arguments to int8_t instead. This eliminates the warnings.

@jrowberg
Copy link
Contributor Author

jrowberg commented Jul 29, 2019 •
edited
Loading

Additional change included; a rare but possible null pointer dereference occurs when a String object is destroyed after either a failed initialization or intentional invalidation. Adding a simple if (buffer) test before freeing it avoids this failure case.

(Also made a similar pull request on the samd core)

@matthijskooijman
Copy link
Collaborator

I wonder if the String changes would be better placed in the https://github.com/arduino/ArduinoCore-API repository? @facchinm can probably confirm?

@jrowberg
Copy link
Contributor Author

I wonder if the String changes would be better placed in the https://github.com/arduino/ArduinoCore-API repository? @facchinm can probably confirm?

Ah, probably so. It seems to be the source for both. I wasn't aware of its existence. I'll be happy to revert that change (if necessary) and submit a new pull request on that repo.

@matthijskooijman
Copy link
Collaborator

It seems to be the source for both

Well, I think it will be the source for both, but it is not yet AFAIK. But I think it will become the source for all cores, so having the change there is certainly a good idea too.

@facchinm
Copy link
Member

I confirm, the API repository should become the base for all future cores so the best place to apply the fix is surely there. I'm not sure about when we'll be able to switch AVR and SAMD core to the new infrastructure but I hope soon 🙂

@per1234
Copy link
Contributor

per1234 commented Aug 6, 2019

jrowberg's PR in ArduinoCore-API as discussed above: arduino/ArduinoCore-API#38

@aentinger aentinger merged commit ffeca15 into arduino:master Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facchinm facchinm facchinm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jrowberg @matthijskooijman @facchinm @per1234 @aentinger